Skip to content

Allow to create a pool with NULL params - #1409

Merged
lukaszstolarczuk merged 2 commits into
oneapi-src:mainfrom
KFilipek:create_with_null
Jul 8, 2025
Merged

Allow to create a pool with NULL params#1409
lukaszstolarczuk merged 2 commits into
oneapi-src:mainfrom
KFilipek:create_with_null

Conversation

@KFilipek

@KFilipek KFilipek commented Jul 3, 2025

Copy link
Copy Markdown
Contributor

Description

Checklist

  • Code compiles without errors locally
  • All tests pass locally
  • CI workflows execute properly
  • CI workflows, not executed per PR (e.g. Nightly), execute properly
  • New tests added, especially if they will fail without my changes
  • Added/extended example(s) to cover this functionality
  • Extended the README/documentation
  • All newly added source files have a license
  • All newly added source files are referenced in CMake files
  • Logger (with debug/info/... messages) is used
  • All API changes are reflected in docs and def/map files, and are tested

@KFilipek
KFilipek requested a review from a team as a code owner July 3, 2025 10:24
@KFilipek KFilipek self-assigned this Jul 3, 2025
bratpiorka
bratpiorka reviewed Jul 3, 2025
View reviewed changes
Comment thread include/umf/pools/pool_disjoint.h Outdated
bratpiorka
bratpiorka reviewed Jul 3, 2025
View reviewed changes
Comment thread src/pool/pool_jemalloc.c Outdated
bratpiorka
bratpiorka reviewed Jul 3, 2025
View reviewed changes
Comment thread test/test_pool_null_params.cpp Outdated
bratpiorka
bratpiorka reviewed Jul 3, 2025
View reviewed changes
Comment thread test/test_pool_null_params.cpp Outdated
bratpiorka
bratpiorka reviewed Jul 3, 2025
View reviewed changes
Comment thread test/test_pool_null_params.cpp Outdated
lplewa
lplewa requested changes Jul 3, 2025
View reviewed changes
Comment thread include/umf/pools/pool_jemalloc.h Outdated
Comment thread include/umf/pools/pool_scalable.h Outdated
@KFilipek
KFilipek force-pushed the create_with_null branch from d14149a to 45c78ee Compare July 3, 2025 12:00
bratpiorka
bratpiorka reviewed Jul 3, 2025
View reviewed changes
Comment thread test/test_pool_null_params.cpp Outdated
bratpiorka
bratpiorka reviewed Jul 3, 2025
View reviewed changes
Comment thread test/test_pool_null_params.cpp Outdated
bratpiorka
bratpiorka reviewed Jul 3, 2025
View reviewed changes
Comment thread src/pool/pool_disjoint.c
@KFilipek
KFilipek requested review from bratpiorka and lplewa July 3, 2025 12:37
@KFilipek
KFilipek force-pushed the create_with_null branch from 45c78ee to 675f786 Compare July 3, 2025 12:38
@KFilipek
KFilipek force-pushed the create_with_null branch 2 times, most recently from eba9ea7 to 2c04d26 Compare July 3, 2025 13:52
lukaszstolarczuk
lukaszstolarczuk reviewed Jul 3, 2025
View reviewed changes
Comment thread test/test_pool_null_params.cpp Outdated
Comment thread test/CMakeLists.txt Outdated
Comment thread test/test_pool_null_params.cpp Outdated
Comment thread test/CMakeLists.txt Outdated
Comment thread src/pool/pool_jemalloc.c Outdated
@KFilipek
KFilipek force-pushed the create_with_null branch 2 times, most recently from fc88217 to 40739be Compare July 3, 2025 15:00
@KFilipek
KFilipek requested a review from lukaszstolarczuk July 3, 2025 15:01
@KFilipek
KFilipek force-pushed the create_with_null branch 2 times, most recently from 40d794c to 5c028ae Compare July 4, 2025 09:13
lplewa
lplewa reviewed Jul 4, 2025
View reviewed changes
Comment thread src/pool/pool_disjoint.c Outdated
@KFilipek
KFilipek force-pushed the create_with_null branch from 5c028ae to e9d89b6 Compare July 4, 2025 16:19
@KFilipek
KFilipek requested a review from lplewa July 7, 2025 08:45
lplewa
lplewa reviewed Jul 7, 2025
View reviewed changes
Comment thread src/pool/pool_jemalloc.c Outdated
Comment on lines +434 to +446
int n_arenas_set_from_params = 0;
int err;
const umf_jemalloc_pool_params_t *jemalloc_params = params;
const umf_jemalloc_pool_params_t *jemalloc_params;

size_t n_arenas = 0;
if (jemalloc_params) {
n_arenas = jemalloc_params->n_arenas;
n_arenas_set_from_params = 1;
// If params is NULL, use default values
umf_jemalloc_pool_params_t default_params = {}; // Will be calculated later

if (!params) {
jemalloc_params = &default_params;
} else {
jemalloc_params = params;
}
size_t n_arenas = jemalloc_params->n_arenas;
int n_arenas_set_from_params = (params != NULL);

@lplewa lplewa Jul 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

@bratpiorka bratpiorka Jul 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why what? please be more specific in reviews! :)

@lplewa lplewa Jul 7, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this is very specific question which i asked - why did you made this changes?

@KFilipek KFilipek Jul 7, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the original implementation, there was a variable definition that would later change. Here is a definition that relies on the value provided by the user (NULL or not) and set proper value.

@KFilipek KFilipek Jul 8, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted

@lukaszstolarczuk lukaszstolarczuk mentioned this pull request Jul 7, 2025
Closed
@KFilipek
KFilipek requested a review from lplewa July 7, 2025 16:33
@KFilipek
KFilipek force-pushed the create_with_null branch from 3dc3f45 to 2e22aa4 Compare July 8, 2025 10:32
@lukaszstolarczuk
lukaszstolarczuk merged commit 98b0170 into oneapi-src:main Jul 8, 2025
77 checks passed
@KFilipek
KFilipek deleted the create_with_null branch July 8, 2025 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bratpiorka bratpiorka bratpiorka approved these changes
@lplewa lplewa lplewa approved these changes
@lukaszstolarczuk lukaszstolarczuk lukaszstolarczuk approved these changes

Assignees

@KFilipek KFilipek

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@KFilipek @bratpiorka @lplewa @lukaszstolarczuk